-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(spanner): set client wide ReadOptions, ApplyOptions, and TransactionOptions #6486
feat(spanner): set client wide ReadOptions, ApplyOptions, and TransactionOptions #6486
Conversation
// order of precedence. | ||
func (co CommitOptions) merge(opts CommitOptions) CommitOptions { | ||
return CommitOptions{ | ||
ReturnCommitStats: co.ReturnCommitStats || opts.ReturnCommitStats, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could not figure out a good way to merge the ReturnCommitStats...
3e73ce2
to
f2ae2ad
Compare
f2ae2ad
to
70527c2
Compare
@rahul2393 Would you review this PR when you have time? 🙂 |
@shuheiktgw Thanks for the contribution, Can you please add relevant tests for this? |
d1eaba8
to
f04a6e7
Compare
Thanks for your comment, @rahul2393! I've added unit tests, so would you review the PR? 🙏 |
Please resolve conflicts @shuheiktgw |
…_client_wide_options
Sure, I've resolved the conflict so would you take a look? 🙂 @rahul2393 |
Currently, if we want to set ReadOptions, ApplyOptions, or TransactionOptions, we need to pass those options as a parameter each time we call the related functions. That is tedious and hard to maintain. Instead, I'd like to specify those options as part of the ClientConfig and use them as default values as the QueryOptions already does. Thanks for your review 🙂